Fixes and improvements in SOLPSSimulation, SOLPSMesh and related functions #35

vsnever · 2020-09-04T17:38:23Z

This PR addresses the issues #8, #20, #31, #33, #34, #36 and #37.

Issue #8 is addressed in the following way:

Safe to use SOLPSFunction2D and SOLPSVectorFunction2D classes are implemented. They replace SOLPSFunction3D and SOLPSVectorFunction3D. For 3D, use AxisymmetricMapper(SOLPSFunction2D) and VectorAxisymmetricMapper(SOLPSVectorFunction2D).
SOLPSMesh and SOLPSSimulation can now be initialised without modification of hidden attributes;
SOLPSMesh has new attributes for basis vectors, cell connection areas, indices of neighbouring cells and new functions to_poloidal() and to_cartesian() for converting vectors defined on a grid from/to (poloidal, radial)/(R, Z);
species_list in SOLPSSimulation is a list of (name, charge) pairs now instead of a list of strings;
Setting b_field and velocities vectors in curvilinear poloidal coordinates (e_pol, e_rad, e_tor) automaticaly sets b_field_cylindrical and velocities_cylindrical (e_R, e_φ, e_Z) and vice versa (note: e_tor = -e_φ);
Incorrect calculation of basis vectors using cell centres is fixed (basis vectors are defined by left and bottom faces of the cell);
The code for mesh.triangles and mesh.triangle_to_grid_map creation is vectorised (as all other code wherever possible);
SOLPSSimulation now has setters for its properties;
load_solps_from_mdsplus(), load_solps_from_raw_output() and load_solps_from_balance() do not brake SOLPSSimulation interface anymore;
load_solps_from_mdsplus() and load_solps_from_raw_output() are made more robust. Now both support B2 standalone runs. Error handling is added to load_solps_from_mdsplus() for some optional attributes that may not be present on the server;
Other small improvements.

Issue #20 is fixed. *_f2d() and *_f3d() interpolators are added for plasma parameters. For vectors _carteisan suffix is used instead of _f3d.

Issue #31 is fixed. Now if the neutral densities from Eirene are available, they replace the neutral densities from B2.

Issue #33 is fixed using lookup_isotope() and lookup_element() methods. The plasma composition is defined from nuclear charge, atomic mass number and electrical charge. This is more robust than parsing a string with species names.

Issue #34 is addressed by parsing the following additional quantities in load_solps_from_mdsplus() and load_solps_from_raw_output():

Ion temperature;
magnetic field (previously was read only from MDS);
atom/ion fluxes (B2), neutral atom densities (Eirene, if available),
neutral atom fluxes (Eirene, if available);
neutral atom temperatures (Eirene, if available).

The fluxes are not stored in SOLPSSimulation, but used to calculate velocities.

Issue #36 is fixed. The parallel velocity and the B2 fluxes (in s^-1) defined on cell faces are used in b2_flux_to_velocity() to calculate velocities at cell centres. If neutral atom fluxes from Eirene (in m^-2 s^-1) defined at cell centres are available, they are used in eirene_flux_to_velocity() replacing the B2 ones.

Issue #37 is fixed. Now all arrays are row-major. However, the indexing is inverted. The correct index order now is: (radial, poloidal) or (species, radial, poloidal) for scalars and (vector component, radial, poloidal) or (species, vector component, radial, poloidal) for vectors. This should improve the performance in typical workflows.

Remaining issues:

Electron velocities are still not read from SOLPS data.
Molecular and total H-alpha emission are still not read from SOLPS data.

… to SOLPSSimulation, other fixes.

vsnever · 2020-09-05T00:07:44Z

Just updated the code by adding ion and neutral atom temperatures. The latter is not available in stand-alone B2.5 simulations.

vsnever · 2020-09-05T22:41:13Z

I renamed this PR because now it addresses not just #31 but in some degree #8 too.

Replacing the algorithm for poloidal basis vectors calculation in SOLPSMesh was more a bug fix than an improvement. The SOLPS mesh have break points due to the separatrix. In the old algorithm, which calculated the basis vectors using the cell centres, the basis vectors had wrong values at these break points.

vsnever · 2020-09-06T13:22:51Z

Just noticed that conversion from poloidal to Cartesian coordinates in mdsplus.py is incorrect. It's for orthogonal basis, but SOLPS uses curvilinear coordinates.

vsnever · 2020-09-06T14:05:31Z

Just noticed that conversion from poloidal to Cartesian coordinates in mdsplus.py is incorrect. It's for orthogonal basis, but SOLPS uses curvilinear coordinates.

Sorry, I'm wrong here. It converts to Cartesian, so it's OK.

…on on assignment to b_field and velocities in SOLPSSimulation.

mattngc · 2020-09-06T16:43:51Z

Hey Vlad, I don't have any strong opinions on these improvements. I don't use this module as much at the moment, so I think opinions from the main users are more important. I'll let @jacklovell and @Mateasek give feedback on these changes.

…_simulation_files. Removed radial_particle_flux and radial_area attributes from SOLPSSimulation.

…ort of B2.5 stand-alone simulations

…tations.

vsnever · 2020-09-07T17:40:25Z

Hey Vlad, I don't have any strong opinions on these improvements. I don't use this module as much at the moment, so I think opinions from the main users are more important. I'll let @jacklovell and @Mateasek give feedback on these changes.

Hi Matthew, thanks for reply. I need a couple more days to get this PR ready for review.

…fy future implementation of cherab#20 via setters.

…ier future implementation of cherab#20.

vsnever · 2020-09-09T13:44:32Z

In the first post, I summaraised the changes made in this PR and marked it ready for review.

Hi @jacklovell and @Mateasek, I am awaiting your verdict).

Hello @jrh-ccfe, could you please share any balance.nc file, so I could improve load_solps_from_balance(). I have access to Heimdall cluster, so a path to the file will be enough.

cherab/solps/eirene/parser/fort44.py

cherab/solps/formats/balance.py

cherab/solps/solps_plasma.py

…*() methods, added setters instead.

…and *_f3d() interpolators.

…t.44. Made fort.44 version check strict again.

vsnever · 2020-09-15T23:20:56Z

I updated balance.py and revert the change in the fort44 parser.

jacklovell

Thanks for this Vlad: it's a huge amount of great work and a big improvement to the module overall. I've made some in-line comments on the code, but generally it is of a very good standard.

I've run the 3 demo scripts we currently have on a limited set of runs that I have access to and they seem to produce mostly the same results as before. I expect any small differences are a result of bug fixes. Have you done regression testing on any other SOLPS cases you have?

There's a few places in the code with large commented-out blocks: these can be removed if they won't be added back in later. I'm thinking in particular in solps_plasma.py, where there are several commented-out plotting methods which should live in demo scripts instead.

Also, considering the amount of change here it would be worth adding to the changelog while the changes are still fresh. I'll defer to @CnlPepper on whether this is a big enough change to warrant a 1.2 (or even a 2.0) release, or whether a 1.1.1 would be suitable.

cherab/solps/b2/parse_b2_block_file.py

cherab/solps/eirene/parser/fort44_2013.py

cherab/solps/eirene/parser/fort44_2017.py

cherab/solps/eirene/eirene.py

jacklovell · 2020-09-18T15:28:27Z

cherab/solps/solps_plasma.py

-        if self._total_rad is None:
-            raise RuntimeError("Total radiation not available for this simulation")
+        if self._total_radiation is None:
+            raise RuntimeError("Total radiation not available for this simulation.")
        else:


else is not necessary after a raise. This applies to all the total_radiation*, b_field* and eirene_simulation properties.

This code was before me, some properties threw errors when not initialized and some did not. I think, they should not throw errors. If a property is required for a function to work, that function must check if the value is set and throw a RuntimeError if not. I've removed these raise statements from properties and instead added them to functions that don't work without these properties.

cherab/solps/solps_plasma.py

jacklovell · 2020-09-18T15:36:28Z

cherab/solps/solps_plasma.py

-        for k, sp in enumerate(self.species_list):
+        if self.electron_velocities_cartesian is None:
+            print('Warning! No electron velocity data available for this simulation.')
+            electron_velocity = lambda x, y, z: Vector3D(0, 0, 0)


This should be a ConstantVector3D object, not a lambda function.

jacklovell · 2020-09-18T15:37:12Z

cherab/solps/solps_plasma.py

-            if isinstance(self.velocities_cartesian, np.ndarray):
-                velocity = SOLPSVectorFunction3D(tri_index_lookup, tri_to_grid, self.velocities_cartesian[:, :, k, :])
+            if self.velocities_cartesian is not None:
+                velocity = self.velocities_cartesian[k]
            else:
                velocity = lambda x, y, z: Vector3D(0, 0, 0)


This should be a ConstantVector3D object, not a lambda function.

jacklovell · 2020-09-18T15:41:53Z

cherab/solps/solps_plasma.py

+        else:
+            electron_velocity = self.electron_velocities_cartesian
+        plasma.electron_distribution = Maxwellian(self.electron_density_f3d, self.electron_temperature_f3d, electron_velocity, electron_mass)
+


Trailing whitespace.

jacklovell · 2020-09-18T15:57:40Z

One other comment: if Axisymmetric wrappers of SOLPSFunction2D and SOLPSVectorFunction2D replace the corresponding 3D functions, perhaps we should remove the latter from the module entirely. The 3D functions are still present in this PR.

CnlPepper · 2020-09-18T16:22:24Z

One other comment: if Axisymmetric wrappers of SOLPSFunction2D and SOLPSVectorFunction2D replace the corresponding 3D functions, perhaps we should remove the latter from the module entirely. The 3D functions are still present in this PR.

I agree, it would be best to remove redundant code.

CnlPepper · 2020-09-18T16:36:10Z

Also, considering the amount of change here it would be worth adding to the changelog while the changes are still fresh. I'll defer to @CnlPepper on whether this is a big enough change to warrant a 1.2 (or even a 2.0) release, or whether a 1.1.1 would be suitable.

While there are large API changes to the SOLPSSimulation API, the core functionality for loading and generating Plasma objects
and the object layout are essentially the same. It is definitely a new minor release, possibly a major release.

The arguments for a major/minor version increment would be the degree of impact to the main usage of the package. This one lies on the border I suspect. As the active users of this package, you would be best placed to assess the impact on users and then decide if you want to go for v2.0.0 or v1.2.0.

Please do add a changelog. I would recommend getting into the habit of checking if it needs updating with every pull request.

Mateasek · 2020-09-18T16:51:34Z

cherab/solps/mesh_geometry.py

+                             must be 3 dimensional. Example shape is (4 x 32 x 98).
+                             In SOLPS notation: left/right - poloidal prev./next, bottom/top - radial prev./next.
+                             Cell indexing starts with 0 and -1 means no neighbour.
+    :param ndarray neighbix: Array of radial indeces of neighbouring cells in order: left, bottom, right, top,


shouldn't it be neighbiy?

Mateasek · 2020-09-18T17:54:17Z

cherab/solps/solps_plasma.py

+
+        self._eirene = value
+
+    def b2_flux_to_velocity(self, poloidal_flux, radial_flux, parallel_velocity):


Wouldn't it be better to make this method a standalone instead of class method, like it is in the case of EFITEquilibrium class? This class is becoming quite big, it would help decreasing it. Also the method itself could be useful/used for various purposes. The same could be done with eirene_flux_to_velocity.

Mateasek · 2020-09-18T18:00:27Z

That is a large amount of work @vsnever, very nice. Thanks for all the work you put into it, especially when SOLPS is not the easiest code to understand and read! I would like to propose one more change, since this is quite major rewrite. I would suggest also removing the commented plotting methods from the SOLPSimulation class. I think the plots would be better done outside in a separate file, as it is with equilibrium plots in Cherab. I don't mean that you should be also adding it now.

…inside/outside mesh in SOLPSTotalRadiatedPower.

vsnever · 2020-09-19T17:44:16Z

@jacklovell, @Mateasek, @CnlPepper, thank you very much for the code review. I hope I addressed all the issues you raised. For three of them I left the comments (see above). If I did not leave a comment, it means that the issue is fixed exactly as you suggested.

I've run the 3 demo scripts we currently have on a limited set of runs that I have access to and they seem to produce mostly the same results as before. I expect any small differences are a result of bug fixes. Have you done regression testing on any other SOLPS cases you have?

When the data is read from the MDSPlus, the only difference should be in species velocities, because of the bug fix. When the data is read from the balance.nc file, then in addition to velocities, the density of impurity neutral atoms is different, because now it's obtained from the Eirene data. If the data is read from raw simulation files then the density of main plasma neutral atoms is also different because previously it was obtained from B2.

Also, considering the amount of change here it would be worth adding to the changelog while the changes are still fresh. I'll defer to @CnlPepper on whether this is a big enough change to warrant a 1.2 (or even a 2.0) release, or whether a 1.1.1 would be suitable.

I updated the changelog suggesting a 1.2 version.

In addition to the improvements you suggested, I removed redundant check for inside/outside mesh in SOLPSTotalRadiatedPower emitter, and because now this emitter uses only a total_radiation_f3d from SOLPSSimulation, I think that passing the simulation object to this emitter is no longer needed, a Function3D instance is enough.

jacklovell

Looks good to me.

jacklovell · 2020-09-21T11:51:09Z

@CnlPepper @Mateasek are you happy for these changes to be merged?

Mateasek · 2020-09-21T11:52:44Z

Yes, sorry I am new to this, forgot to click on approve. Very nice piece of work!

CnlPepper · 2020-09-21T18:48:44Z

Happy here, nice work!

vsnever · 2020-09-21T19:09:26Z

@jacklovell, @Mateasek, @CnlPepper, thanks a lot!

@jacklovell, please do not close #34 when this is merged. There is still plenty of work to do.

vsnever added 2 commits September 4, 2020 19:31

Removed _popular_species and _species_symbol_map dicts, added setters…

d428e5b

… to SOLPSSimulation, other fixes.

Added ion and nuetral atom temperatures.

9006823

Many fixes and improvements in SOLPSMesh class

1e41cd4

vsnever changed the title ~~Add setters to SOLPSSimulation, remove unsued attributes, improved reading from MDSPlus and raw files~~ Fixes and improvements in SOLPSSimulation, SOLPSMesh and related functions Sep 5, 2020

Corrected mesh basis vector calculation

687e991

vsnever added 2 commits September 6, 2020 18:14

Added poloidal <--> Cartesian conversion to SOLPSMesh. Added conversi…

5dcc0dd

…on on assignment to b_field and velocities in SOLPSSimulation.

Replaced nested loops in _map_data_onto_triangles with vector code.

bdf99da

vsnever added 3 commits September 7, 2020 02:01

Added magnetic files, velocity and neutral temperature reading to raw…

800b878

…_simulation_files. Removed radial_particle_flux and radial_area attributes from SOLPSSimulation.

Added exception handling to load_solps_from_mdsplus() for better supp…

9b0e9c1

…ort of B2.5 stand-alone simulations

Renamed parallel direction to poloidal in SOLPSMesh to match SOLPS no…

b840d3f

…tations.

vsnever added 5 commits September 9, 2020 01:02

Added fluxes to velocity conversion to adress cherab#36.

8a082e7

Removed modification of assigned SOLPSSimulation attributes to simpli…

df6ad34

…fy future implementation of cherab#20 via setters.

Added set_..() and get_..() interfaces to SOLPSSimulation to make eas…

9ce89bb

…ier future implementation of cherab#20.

Made assign_fort44_parser() less strict to adress cherab#32

52a5162

Fixed errors with pickle save/load.

48445d9

vsnever marked this pull request as ready for review September 9, 2020 13:28

CnlPepper reviewed Sep 10, 2020

View reviewed changes

cherab/solps/eirene/parser/fort44.py Outdated Show resolved Hide resolved

CnlPepper reviewed Sep 10, 2020

View reviewed changes

cherab/solps/eirene/parser/fort44.py Outdated Show resolved Hide resolved

CnlPepper reviewed Sep 10, 2020

View reviewed changes

cherab/solps/formats/balance.py Outdated Show resolved Hide resolved

CnlPepper reviewed Sep 10, 2020

View reviewed changes

cherab/solps/solps_plasma.py Show resolved Hide resolved

vsnever added 3 commits September 10, 2020 14:50

Corrected the typo

1e55144

Made all array row-major and inverted indexing. Removed get_*(), set_…

1884289

…*() methods, added setters instead.

Implemented SOLPSFunction2D and SOLPSVectorFunction2D, added *_f2d() …

8953c54

…and *_f3d() interpolators.

vsnever mentioned this pull request Sep 14, 2020

Make fort44 parser to support more versions of EIRENE fort.44 file #32

Open

vsnever added 2 commits September 16, 2020 02:10

eirene.elosm and eirene.edism now resolved over molecules like in for…

1a0b412

…t.44. Made fort.44 version check strict again.

Updated balance.py to match the functionality of mdsplus.py.

1c2a22f

jacklovell requested changes Sep 18, 2020

View reviewed changes

Mateasek reviewed Sep 18, 2020

View reviewed changes

Fixes in response to reviewers comments. Removed redundant check for …

a7202e4

…inside/outside mesh in SOLPSTotalRadiatedPower.

jacklovell approved these changes Sep 21, 2020

View reviewed changes

Mateasek approved these changes Sep 21, 2020

View reviewed changes

jacklovell removed a link to an issue Sep 22, 2020

Use more SOLPS data when available #34

Open

jacklovell merged commit 187be3d into cherab:maintenance/raysect-imports Sep 22, 2020

This was referenced Apr 6, 2021

Look up in cherab.core.atomic.elements instead of using _popular_species and _species_symbol_map dicts #33

Closed

Fix velocity calculation #36

Closed

Invert index order of SOLPS data arrays or make all arrays as column-major #37

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes and improvements in SOLPSSimulation, SOLPSMesh and related functions #35

Fixes and improvements in SOLPSSimulation, SOLPSMesh and related functions #35

vsnever commented Sep 4, 2020 •

edited

Loading

vsnever commented Sep 5, 2020

vsnever commented Sep 5, 2020

vsnever commented Sep 6, 2020

vsnever commented Sep 6, 2020

mattngc commented Sep 6, 2020

vsnever commented Sep 7, 2020

vsnever commented Sep 9, 2020

vsnever commented Sep 15, 2020

jacklovell left a comment •

edited

Loading

jacklovell Sep 18, 2020

vsnever Sep 19, 2020

jacklovell Sep 18, 2020

jacklovell Sep 18, 2020

jacklovell Sep 18, 2020

jacklovell commented Sep 18, 2020

CnlPepper commented Sep 18, 2020

CnlPepper commented Sep 18, 2020

Mateasek Sep 18, 2020

Mateasek Sep 18, 2020

Mateasek commented Sep 18, 2020 •

edited

Loading

vsnever commented Sep 19, 2020

jacklovell left a comment

jacklovell commented Sep 21, 2020

Mateasek commented Sep 21, 2020

CnlPepper commented Sep 21, 2020

vsnever commented Sep 21, 2020


		self._eirene = value

		def b2_flux_to_velocity(self, poloidal_flux, radial_flux, parallel_velocity):

Fixes and improvements in SOLPSSimulation, SOLPSMesh and related functions #35

Fixes and improvements in SOLPSSimulation, SOLPSMesh and related functions #35

Conversation

vsnever commented Sep 4, 2020 • edited Loading

vsnever commented Sep 5, 2020

vsnever commented Sep 5, 2020

vsnever commented Sep 6, 2020

vsnever commented Sep 6, 2020

mattngc commented Sep 6, 2020

vsnever commented Sep 7, 2020

vsnever commented Sep 9, 2020

vsnever commented Sep 15, 2020

jacklovell left a comment • edited Loading

Choose a reason for hiding this comment

jacklovell Sep 18, 2020

Choose a reason for hiding this comment

vsnever Sep 19, 2020

Choose a reason for hiding this comment

jacklovell Sep 18, 2020

Choose a reason for hiding this comment

jacklovell Sep 18, 2020

Choose a reason for hiding this comment

jacklovell Sep 18, 2020

Choose a reason for hiding this comment

jacklovell commented Sep 18, 2020

CnlPepper commented Sep 18, 2020

CnlPepper commented Sep 18, 2020

Mateasek Sep 18, 2020

Choose a reason for hiding this comment

Mateasek Sep 18, 2020

Choose a reason for hiding this comment

Mateasek commented Sep 18, 2020 • edited Loading

vsnever commented Sep 19, 2020

jacklovell left a comment

Choose a reason for hiding this comment

jacklovell commented Sep 21, 2020

Mateasek commented Sep 21, 2020

CnlPepper commented Sep 21, 2020

vsnever commented Sep 21, 2020

vsnever commented Sep 4, 2020 •

edited

Loading

jacklovell left a comment •

edited

Loading

Mateasek commented Sep 18, 2020 •

edited

Loading